upgrade to typescript 2.0 and fix broken build process by FredKSchott · Pull Request #47 · Polymer/polymer-build

FredKSchott · 2016-09-21T17:56:25Z

@usergenic and I ran into problems with gulp-typescript yesterday, this upgrades to the latest 2.0 version and fixes some issues with config, custom_typings, etc.

/cc @usergenic @justinfagnani

justinfagnani · 2016-09-21T18:04:27Z

yay! taking a look

justinfagnani · 2016-09-21T18:07:43Z

test/service-worker_test.js

-        serviceWorker.generateServiceWorker();
-      }, Error.AssertionError, '`project` & `buildRoot` options are required');
+      return serviceWorker.generateServiceWorker()
+        .then(() => { throw new Error('generateServiceWorker() resolved, expected rejection!'); })


I would do Promise inversions as two callback to then(), so that you don't have to worry about the exception you throw in then() flowing into catch(). Then just assert.fail() in the first callback. Alternatively, we have a handy invert() function hangout out around here somewhere...

https://github.com/Polymer/polymer-analyzer/blob/master/src/test/test-utils.ts#L23

nice, I like this.

justinfagnani · 2016-09-21T18:08:31Z

tsconfig.json

-    "src/**/*.ts",
-    "typings/index.d.ts"
-  ],
  "files": [


TS 2.x supports includes and excludes. Use those instead of files.

🎉 nice! It's behavior is a little weird, which is why I hadn't been able to get it working before. Files still need to be referenced in the files array. ¯_(ツ)_/¯

you shouldn't. We don't use files at all in the analyzer

Please try to remove files before submitting. One file is replaced by compilerOptions.lib, the other should go in includes.

👍 got it! Taking out"typings/index.d.ts" was what caused the error I was seeing, but it turns out it was a problem with typings and not typescript.

justinfagnani · 2016-09-21T18:09:11Z

typings.json

@@ -8,7 +8,6 @@
    "merge-stream": "registry:dt/merge-stream#1.0.0+20160313224417",


can you migrate to @types while we're at this?

not all of them are supported yet, but I'll migrate the ones I can

which ones? All of these come from DT, so they should be available in @types

Only DT stuff in their 2.0 branch is in @types. They're using this as a chance to standardize on modern practices.

usergenic

OMG ITS WORKING

usergenic · 2016-09-21T18:41:16Z

TRAVIS Y U NO LIKE?

npm ERR! peerinvalid The package typescript@2.1.0-dev.20160921 does not satisfy its siblings' peerDependencies requirements!
npm ERR! peerinvalid Peer tslint@3.15.1 wants typescript@>=1.7.3

Anything we can do to fix that here too so we have green travis checks?

wrt https://travis-ci.org/Polymer/polymer-build/jobs/161702741

(unless node 4 is no longer unsupported)

…itions

FredKSchott · 2016-09-21T18:58:07Z

@usergenic the typescript@next dev versions were messing with semver matching, which was causing problems with npm2 installations. Switching back to the latest stable version (which we should be on, now that TS 2.0 is stable) fixes it.

FredKSchott · 2016-09-21T18:58:54Z

@justinfagnani PTAL

justinfagnani · 2016-09-21T19:02:35Z

tsconfig.json

-  }
+  "include": [
+    "custom_typings/**/*.ts",
+    "node_modules/@types/**/*.ts",


I don't think you need or want this line. tsc automatically looks for these when you use node resolution

justinfagnani · 2016-09-21T19:04:59Z

tsconfig.json

-    "rewriteTsconfig": true
-  }
+  "include": [
+    "custom_typings/**/*.ts",


@rictic do you remember why we use /// <reference path="../custom_typings/main.d.ts" /> in analyzer.ts rather than include custom_typings in tsconfig? Is it for declarations and dependers?

Yeah, it's for the generated .d.ts declarations file. If you only mention the dependency in your tsconfig then downstream users won't see the custom typings.

I'll add this for the sw-precache custom typing and we can look into publishing it directly to npm. All the others are internal only and shouldn't affect the polymer-build declarations

justinfagnani · 2016-09-21T19:12:29Z

gulpfile.js


 gulp.task('depcheck', () =>
-  depcheck(__dirname, {})
+  depcheck(__dirname, {ignoreMatches: ['@types/**']})


can you comment the ignore?

We should really make that part of the default ignoreMatches

+10, I'll create a PR and comment for now

Already tracking here: depcheck/depcheck#163

justinfagnani · 2016-09-21T19:13:39Z

test/service-worker_test.js

-      }, Error.AssertionError, '`project` & `buildRoot` options are required');
+      return serviceWorker.generateServiceWorker()
+        .then(
+          () => { throw new Error('generateServiceWorker() resolved, expected rejection!'); },


instead of throwing, you can: assert.fail('generateServiceWorker() resolved, expected rejection!')

Any reason to prefer assert.fail over new Error?

The UX. I think an uncaught error is reported different, and tries to display a stacktrace. Here we know it's a plain failure and the stack won't help.

I'm fine with either, the difference seems minor

I'd use fail()

FredKSchott · 2016-09-21T19:58:59Z

@justinfagnani @rictic PTAL

rictic · 2016-09-21T20:00:53Z

src/bundle.ts

-        shellFile.contents = new Buffer(newShellContent);
-      }
+  async _buildBundles(): Promise<Map<string, string>> {
+    let bundles = await this._getBundles();


Here and elsewhere: preferred style is const by default, let only when necessary.

Agreed, but I'd like to keep the scope of this PR small for now. I'll make a separate PR to address this across the repo once this has been merged.

Agreed w/ the agree. In the future, just do the async/await in another PR also :)

rictic · 2016-09-21T20:05:16Z

src/bundle.ts

+        ? urlFromPath(this.root, this.shell)
+        : this.sharedBundleUrl;
+    let sharedDeps = bundles.get(sharedDepsBundle) || [];
+    let promises: Promise<any>[] = [];


Minimize use of any, it's usually a code smell. Would this work?

const promises: Promise<{url: string, contents: string}>[] = []

Agreed, but I'd like to keep the scope of this PR small for now. This diff was only a indentation change due to a move over to async-await above. I'll make a separate PR to address this.

rictic · 2016-09-21T20:06:17Z

tsconfig.json

    "declaration": true,
    "noImplicitAny": true,
    "removeComments": false,
-    "noLib": true,


noLib still makes sense, or

"lib": ["es2017"],

rictic · 2016-09-21T20:07:11Z

typings.json

@@ -8,7 +8,6 @@
    "merge-stream": "registry:dt/merge-stream#1.0.0+20160313224417",


Only DT stuff in their 2.0 branch is in @types. They're using this as a chance to standardize on modern practices.

rictic · 2016-09-21T20:08:58Z

As a code review style note, when possible instead of rewriting existing commits add --fixup commits instead, and you can rebase them right before going to master. It makes it easier for us to see only the new changes.

FredKSchott · 2016-09-21T20:22:01Z

@rictic TIL! I've never used the --fixup feature before, I'll play around with it.

@rictic @justinfagnani thanks for the help so far with all the different TypeScript config options, feeling much more confident in our config setup now.

justinfagnani · 2016-09-21T20:31:31Z

src/bundle.ts

-        shellFile.contents = new Buffer(newShellContent);
-      }
+  async _buildBundles(): Promise<Map<string, string>> {
+    let bundles = await this._getBundles();


Agreed w/ the agree. In the future, just do the async/await in another PR also :)

FredKSchott assigned usergenic and justinfagnani Sep 21, 2016

justinfagnani suggested changes Sep 21, 2016

View reviewed changes

upgrade tslint & typescript for 2.0 and fix broken build process

4eee692

usergenic approved these changes Sep 21, 2016

View reviewed changes

Fred K. Schott added 2 commits September 21, 2016 11:47

use new tsconfig includes syntax

015d56a

migrate vinyl-fs & fs-extra custom type definitions to npm type defin…

f5cada9

…itions

FredKSchott force-pushed the typescript-fix branch from f347fa5 to f5cada9 Compare September 21, 2016 18:48

catch errors within then() call for service-worker tests

18020ae

FredKSchott force-pushed the typescript-fix branch from fe68851 to 18020ae Compare September 21, 2016 18:57

fix silly depcheck error

9e1a193

justinfagnani reviewed Sep 21, 2016

View reviewed changes

Fred K. Schott added 2 commits September 21, 2016 12:56

Add sw-precache custom_typing declaration reference

67c86f8

small fix to test structure

10b4fc2

FredKSchott assigned rictic Sep 21, 2016

generate type declarations via gulp build

75a531f

FredKSchott force-pushed the typescript-fix branch from e792601 to 75a531f Compare September 21, 2016 20:02

rictic suggested changes Sep 21, 2016

View reviewed changes

justinfagnani approved these changes Sep 21, 2016

View reviewed changes

Remove tsconfig files array

9f151bd

FredKSchott merged commit 6d13fac into master Sep 21, 2016

FredKSchott deleted the typescript-fix branch September 21, 2016 21:41

		@@ -8,7 +8,6 @@
		"merge-stream": "registry:dt/merge-stream#1.0.0+20160313224417",

Conversation

FredKSchott commented Sep 21, 2016

Uh oh!

justinfagnani commented Sep 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FredKSchott Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

usergenic left a comment

Choose a reason for hiding this comment

Uh oh!

usergenic commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FredKSchott commented Sep 21, 2016

Uh oh!

FredKSchott commented Sep 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FredKSchott commented Sep 21, 2016

Uh oh!

rictic Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

FredKSchott Sep 21, 2016 •

edited

Loading

usergenic commented Sep 21, 2016 •

edited

Loading

rictic Sep 21, 2016 •

edited

Loading